Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CreateOrUpdateOrgSecret regression introduced in v53 #2817

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

frankywahl
Copy link
Contributor

CreateOrUpdateOrgSecret's API uses string for selected repository IDS while other APIs use int. This fixes the issue locally by implementing its own type.

fixes #2816

@google-cla
Copy link

google-cla bot commented Jun 23, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Comment on lines 160 to 163
putSecret := func(ctx context.Context, url string, eSecret interface{}) (*Response, error) {
req, err := s.client.NewRequest("PUT", url, eSecret)
if err != nil {
return nil, err
}

return s.client.Do(ctx, req, nil)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this inline for now as it's the only place it's used. Happy to pull it out as an unexported function to match the putSecret package function (maybe putOrgSecret). That would require though defining a package type for params (for used as an input argument). I thought it was an overkill for now, so left it as such.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for the inline function here. Let's just please delete lines 160 and 167, and move lines 161-166 down to replace line 170 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - was trying to keep the symmetry with what was previously there. Removed!

@frankywahl frankywahl force-pushed the fix-org-secret branch 2 times, most recently from ac1e88d to bca8d40 Compare June 23, 2023 07:07
@frankywahl frankywahl marked this pull request as ready for review June 23, 2023 14:30
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #2817 (d573d47) into master (3220fa0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2817   +/-   ##
=======================================
  Coverage   98.05%   98.06%           
=======================================
  Files         136      136           
  Lines       12263    12279   +16     
=======================================
+ Hits        12025    12041   +16     
  Misses        162      162           
  Partials       76       76           
Impacted Files Coverage Δ
github/dependabot_secrets.go 100.00% <100.00%> (ø)

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @frankywahl!
Just a few tweaks, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment on lines 160 to 163
putSecret := func(ctx context.Context, url string, eSecret interface{}) (*Response, error) {
req, err := s.client.NewRequest("PUT", url, eSecret)
if err != nil {
return nil, err
}

return s.client.Do(ctx, req, nil)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for the inline function here. Let's just please delete lines 160 and 167, and move lines 161-166 down to replace line 170 below.

github/dependabot_secrets.go Outdated Show resolved Hide resolved
github/dependabot_secrets.go Outdated Show resolved Hide resolved
`CreateOrUpdateOrgSecret`'s API uses `string` for selected repository
IDS while other APIs use `int`. This fixes the issue locally by
implementing its own type
@gmlewis
Copy link
Collaborator

gmlewis commented Jun 23, 2023

Also, in the future, please don't use force-push in PRs to this repo, as it makes reviewing more challenging to see what has changed since the last review.
See CONTRIBUTING.md for more details. Thanks.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @frankywahl !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jun 23, 2023
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jun 27, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Jun 27, 2023

Thank you, @valbeat !
Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Regression in version 0.53 in Dependabot.CreateOrUpdateOrgSecret
3 participants